dsl: Move SubDimension thickness evaluation to the thicknesses themselves #2470

EdCaunt · 2024-10-18T14:12:22Z

Fixes issues with standalone use of SubDimension.symbolic_min or SubDimension.symbolic_max in an operator. Previously variants on

x = Dimension('x')
ix = SubDimension.left('ix', x, 2)
f = Function(name="f", dimensions=(ix,), shape=(5,), dtype=np.int32)

eqns = Eq(f[ix.symbolic_max], 1)

op = Operator(eqns)
op(x_m=0)

would suffer from two issues:

The SubDimension thickness (ix.symbolic_max) would not be concretised from x_ltkn to x_ltkn0
The SubDimension thickness would never be given a value by _arg_values unless the user explicitly passed it to the operator (and even then, it would not be adjusted to reflect any MPI decomposition)

These are rectified by this PR.

FabioLuporini

Looks like a very nice clean up!

FabioLuporini · 2024-10-18T14:32:12Z

devito/operator/operator.py

@@ -640,6 +642,11 @@ def _prepare_arguments(self, autotune=None, **kwargs):
        for d in reversed(toposort):
            args.update(d._arg_values(self._dspace[d], grid, **kwargs))

+        # Process SubDimensionThicknesses
+        for p in self.parameters:


I think this should be triggered from within SubDimension._arg_values itself

No, the issue is that the SubDimension isn't necessarily available to the operator. You can craft legal Operators which only contain the thickness, not the parent subdimension, hence the reworking in this PR

Why isn't it caught by self.input 's default line 487

Assuming "it" means the subdomain, you can craft a legal operator as in the MFE at the top of this PR where the Operator never sees the SubDimension, only its thickness symbols

I don't mean the subdomain no, I mean the thickness, you're saying you can create an operator where the Thickness (p here) is in op.parameters but is not in op.inputs?

If it's the case, i.e if it is not in inputs, then it should not need to be in args since it's not an input.
If it's not the case, the line 487 processes all inputs and should cover that case already

I tried it, and this is not the case

How is that possible? It's clearly in parameters since you added this so if you add is_Input and it doesn't catch it then something is broken

There's no grid in the kwargs at the point where _arg_values is called on the inputs, which makes it no-op. It also needs to be done before objects get _arg_values called on them iirc, hence the current position of this loop.

You could reorder _prepare_arguments to process the grid earlier, but my functions-on-subdomains branch has a bunch of reworking here and fixing the merge conflicts will be a pain, so best to stick a pin in it and fix down the line imo.

Can you open an issue about it then

I also don't understand this thing honestly. We need an issue and ideally a comment on top of this loop explaining what's going on and what should actually happen instead. You can do it in another branch as it's a minor thing

devito/types/dimension.py

FabioLuporini · 2024-10-18T14:33:05Z

devito/types/dimension.py

@@ -536,6 +535,81 @@ def _arg_check(self, *args, **kwargs):
 # The Dimensions below are exposed in the user API. They can only be created by
 # the user

+class SubDimensionThickness(DataSymbol):
+    """A DataSymbol to represent a thickness of a SubDimension"""


Thought we didn't do those on single line docstrings?

devito/types/dimension.py

codecov · 2024-10-18T14:40:38Z

Codecov Report

Attention: Patch coverage is 97.03704% with 4 lines in your changes missing coverage. Please review.

Project coverage is 87.26%. Comparing base (2f4f80f) to head (6e7c017).
Report is 21 commits behind head on master.

Files with missing lines	Patch %	Lines
devito/ir/equations/algorithms.py	81.81%	1 Missing and 1 partial ⚠️
devito/types/dimension.py	97.82%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2470      +/-   ##
==========================================
+ Coverage   87.22%   87.26%   +0.04%     
==========================================
  Files         238      238              
  Lines       45258    45278      +20     
  Branches     4019     4022       +3     
==========================================
+ Hits        39475    39512      +37     
+ Misses       5103     5085      -18     
- Partials      680      681       +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

mloubout

some mnior comments

devito/types/dimension.py

devito/ir/equations/algorithms.py

devito/types/dimension.py

review-notebook-app · 2024-11-04T15:48:40Z

Check out this pull request on

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

tests/test_caching.py

mloubout · 2024-11-07T14:02:45Z

tests/test_caching.py

+        assert len(_SymbolCache) == init_cache_size + 4
+        # Delete the grid and check that all symbols are subsequently garbage collected
+        del grid
+        for n in (10, 3, 0):


Is 10 init_cache_size ? If so just

del grid assert len(_SymbolCache) == init_cache_size

And everything was deleted that's that's needed to check

Init cache size is 11 (and consists of a different set of symbols)

FabioLuporini · 2024-11-11T10:13:25Z

devito/operator/operator.py

@@ -640,6 +642,11 @@ def _prepare_arguments(self, autotune=None, **kwargs):
        for d in reversed(toposort):
            args.update(d._arg_values(self._dspace[d], grid, **kwargs))

+        # Process SubDimensionThicknesses
+        for p in self.parameters:


I think if you add is_Input = True to class Thickness, then you don't need this extra loop

for example https://github.com/devitocodes/devito/blob/master/devito/types/constant.py#L42

devito/types/dimension.py

FabioLuporini · 2024-11-11T10:16:12Z

devito/types/dimension.py

+    def _arg_finalize(self, *args, **kwargs):
+        return {}
+
+    def _arg_apply(self, *args, **kwargs):


drop this, irrelevant

A no-op _arg_check, _arg_finalize, and _arg_apply are required for _prepare_arguments

this should be fixed at the superclass site then, somehow, but fine..

FabioLuporini · 2024-11-11T10:16:22Z

devito/types/dimension.py

+
+        return {self.name: tkn}
+
+    def _arg_finalize(self, *args, **kwargs):


drop this, irrelevant

devito/types/dimension.py

…ckness

FabioLuporini · 2024-11-13T08:50:23Z

Merged, thanks

EdCaunt requested review from FabioLuporini, mloubout, georgebisbas and ZoeLeibowitz October 18, 2024 14:24

FabioLuporini reviewed Oct 18, 2024

View reviewed changes

mloubout reviewed Oct 18, 2024

View reviewed changes

devito/types/dimension.py Outdated Show resolved Hide resolved

devito/types/dimension.py Outdated Show resolved Hide resolved

georgebisbas reviewed Oct 19, 2024

View reviewed changes

devito/ir/equations/algorithms.py Outdated Show resolved Hide resolved

devito/types/dimension.py Outdated Show resolved Hide resolved

devito/types/dimension.py Show resolved Hide resolved

EdCaunt force-pushed the tkns branch from 1d056ab to 4182ae3 Compare October 28, 2024 13:21

EdCaunt force-pushed the tkns branch from 05c5f5c to 405ed78 Compare November 4, 2024 15:48

mloubout reviewed Nov 5, 2024

View reviewed changes

tests/test_caching.py Show resolved Hide resolved

mloubout added the compiler label Nov 7, 2024

mloubout reviewed Nov 7, 2024

View reviewed changes

FabioLuporini reviewed Nov 11, 2024

View reviewed changes

EdCaunt added 15 commits November 11, 2024 11:26

dsl: Adjust how SubDimensions are created

90cd794

dsl: Tweaks to subdimensions

6469359

misc: Further investigations

549af29

misc: Reworking SubDomainThickness

e83e671

misc: Further SubDomain fiddling

59136ca

compiler: Started moving SubDimension arg vals to SubDimensionThickness

1cf8116

compiler: Moved compiler gubbins from SubDimension to SubDimensionThi…

f45ee73

…ckness

tests; Fix up tests and update concretisation functionality

d5b16c2

tests: Fix up symbolics tests

e2eed6a

dsl: Continue refactoring SubDimensions

f11d50d

compiler: Moved thickness arg vals in operator

5c5ee9d

compiler: Remove leftovers from operator

2e02924

dsl: Rework SubDimension types for better inheritance

62cd507

misc: Fix up class names

3805333

misc: Adjust tests and tidy variable names for clarity

a5e42a9

EdCaunt added 5 commits November 11, 2024 11:26

tests: Tweak caching tests to reflect lazy touch of Dimension bounds

26edbc6

misc: Tidy up

3939b06

examples: Update printed code in examples to reflect SymPy ordering

a6dd6b0

tests: Update caching test and fix minor decomposition bug

abf00f8

misc: Tidy up

6e7c017

EdCaunt force-pushed the tkns branch from abb0b72 to 6e7c017 Compare November 11, 2024 11:26

mloubout approved these changes Nov 12, 2024

View reviewed changes

FabioLuporini approved these changes Nov 13, 2024

View reviewed changes

FabioLuporini merged commit 2f18ab8 into master Nov 13, 2024
31 checks passed

FabioLuporini deleted the tkns branch November 13, 2024 08:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dsl: Move SubDimension thickness evaluation to the thicknesses themselves #2470

dsl: Move SubDimension thickness evaluation to the thicknesses themselves #2470

EdCaunt commented Oct 18, 2024

FabioLuporini left a comment

FabioLuporini Oct 18, 2024

EdCaunt Oct 18, 2024

mloubout Nov 7, 2024

EdCaunt Nov 7, 2024

mloubout Nov 7, 2024

EdCaunt Nov 11, 2024

mloubout Nov 11, 2024

EdCaunt Nov 12, 2024

mloubout Nov 12, 2024

FabioLuporini Nov 13, 2024

FabioLuporini Oct 18, 2024

EdCaunt Oct 18, 2024

codecov bot commented Oct 18, 2024 •

edited

Loading

mloubout left a comment

review-notebook-app bot commented Nov 4, 2024

mloubout Nov 7, 2024

EdCaunt Nov 7, 2024

FabioLuporini Nov 11, 2024

FabioLuporini Nov 11, 2024

EdCaunt Nov 11, 2024

FabioLuporini Nov 13, 2024

FabioLuporini Nov 11, 2024

FabioLuporini commented Nov 13, 2024


		return {self.name: tkn}

		def _arg_finalize(self, args, *kwargs):

dsl: Move SubDimension thickness evaluation to the thicknesses themselves #2470

dsl: Move SubDimension thickness evaluation to the thicknesses themselves #2470

Conversation

EdCaunt commented Oct 18, 2024

FabioLuporini left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Oct 18, 2024 • edited Loading

Codecov Report

mloubout left a comment

Choose a reason for hiding this comment

review-notebook-app bot commented Nov 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FabioLuporini commented Nov 13, 2024

codecov bot commented Oct 18, 2024 •

edited

Loading